Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added EdgeWeightedDigraph #575

Merged
merged 2 commits into from
Jan 10, 2024
Merged

Conversation

RaiyanC
Copy link
Contributor

@RaiyanC RaiyanC commented Mar 29, 2023

This pull request effects:

  • doc/z-chap5.xml: This adds the relevant Edge Weights heading with 2 labels: "EdgeWeights" and "EdgeWeightedDigraph"
  • gap/doc.g: Adding weights.xml to the list of DIGRAPHS_DocXMLFiles
  • init.g: Adding weights.gd to the packages to be read in
  • Read.g: Adding weights.gi to the packages to be read in
  • tst/testinstall.tst: Added a very basic test to test creation of EdgeWeightedDigraph
  • doc/weights.xml: Added documentation
  • gap/weights.gd: Added headers for functions implemented in weights.gi
  • gap/weights.gi: Implemented EdgeWeightedDigraph
  • tst/standard/weights.tst: Added tests for EdgeWeightedDigraph creation

@mtorpey
Copy link
Collaborator

mtorpey commented Mar 30, 2023

@RaiyanC: Looks like issues in the manual: see a summary here.

You can reproduce all this locally with DigraphsTestManualExamples(); or at least I could!

The first thing is to note that generally tests only give the first line of an error, so

gap> b[1][1] := 534;
Error, List Assignment: <list> must be a mutable list
not in any function at *stdin*:39
type 'quit;' to quit to outer loop    

should actually just be

gap> b[1][1] := 534;
Error, List Assignment: <list> must be a mutable list

Also watch out for trailing whitespace, e.g. weights.xml line 28, which is 96 characters long (mostly just spaces at the end of the line).

Fix these, and see what else pops up. If you can get DigraphsTestManualExamples() passing locally, then most of this stuff should be fixed in the pull request.

Reminder: you can update this pull request by simply pushing more commits to your fork's AddingEdgeWeightedDigraph branch.

@RaiyanC
Copy link
Contributor Author

RaiyanC commented Mar 31, 2023

Added EdgeWeights attribute, EdgeWeightedDigraph global function with additional utility functions as well as documentation, manual examples and testing.

@mtorpey mtorpey self-assigned this Apr 5, 2023
Copy link
Collaborator

@mtorpey mtorpey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No big issues here, but a few things to change. We can also talk!

doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Show resolved Hide resolved
gap/digraph.gi Outdated Show resolved Hide resolved
gap/digraph.gi Outdated Show resolved Hide resolved
gap/weights.gd Show resolved Hide resolved
@mtorpey
Copy link
Collaborator

mtorpey commented Apr 11, 2023

@RaiyanC: you could merge or rebase on main now. I pushed a quick fix to that bug you discovered.

Copy link
Collaborator

@mtorpey mtorpey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor last changes from me!

doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
Copy link
Member

@james-d-mitchell james-d-mitchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, some minor things that should be fixed before merging.

doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Outdated Show resolved Hide resolved
doc/weights.xml Show resolved Hide resolved
gap/weights.gi Outdated Show resolved Hide resolved
gap/weights.gi Outdated Show resolved Hide resolved
gap/weights.gi Outdated Show resolved Hide resolved
gap/weights.gi Outdated Show resolved Hide resolved
tst/standard/weights.tst Show resolved Hide resolved
@mtorpey
Copy link
Collaborator

mtorpey commented Jan 10, 2024

I'm fixing this up to hopefully merge this week. Sorry we've waited so long!

@mtorpey
Copy link
Collaborator

mtorpey commented Jan 10, 2024

I'm satisfied that this passes, and that all @james-d-mitchell's last suggestions have been acted on.

Suggest merging.

@mtorpey mtorpey merged commit a9597ff into digraphs:main Jan 10, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants